LORIS-MRI tooling configuration #1170

maximemulder · 2024-08-30T15:34:01Z

PR Description

This PR adds a tooling configuration to LORIS-MRI via pyproject.toml (PEP 518).

This PR configures three tools :

Ruff, a Python linter
Pyright, a Python type checker
Pytest, a Python testing library

Thoughts and things to discuss at the next meeting :

Location of the `pyproject.toml` file

Usually, Python projects have the following architecture

src/
    foo.py
tests/
    test_foo.py
pyproject.toml

We do not use this architecture, so the two locations to place pyproject.toml would either be in the root directory, or in the python directory. I propose to use the root directory, because the tools read the pyproject.toml file to get their configuration when running them from the command line, and having the file in the root directory allows to run these tools from there instead of checking out other directories, which is more pratical IMO.

Ruff

Ruff is a Python linter and formatter aimed at replacing other tools like Flake8 and Black. It is still relatively new but is seeing a lot of adoption, including in several of our dependencies like Pydicom or scikit-learn.

The advantages of Ruff over Flake8 are these ones:

Speed, Ruff lints our entire codebase in less than a second.
IDE integration, Ruff has a first-party VSCode extension.
Configuration, Ruff uses the standard pyproject.toml file for its configuration, whereas Flake8 uses its own configuration file or requires an extension to use pyproject.toml.
Better lints, Ruff includes many lints natively, whereas Flake8 requires plugin for some lints (like sorting imports for instance).
Ruff as a more active development than Flake8 according to their commit history.

Pyright

Pyright is a static type checker for Python developed by Microsoft. It is an alternative to Mypy, the official community Python type checker. Both of these type checkers are good, and can in fact be used simultaneously. The advantages of Pyright are the following:

It is used by Pylance, the VSCode Python language server.
It has a better default configuration for our project IMO (return type inference !).
It is faster than Mypy (not important but still noticeable IMO).

Pytest

Pytest is a popular Python testing library, which we will use for our unit and integration tests. This PR does not add any test but adds it to our dependencies and provides a place to configure the tool.

Dependencies in `pyproject.toml`

pyproject.toml also also allows to configure a project dependencies (replacing requirements.txt). However, installing dependencies via pyproject.toml also requires to "build" the project (which is mostly copying the source files and putting a version on it as far as I understand it), which we do not really have any use for now IMO. There are some workarounds (Poetry notably) but they complicate the setup IMO. So I did not touch the dependencies management in this PR.

Dev dependencies vs prod dependencies

As is currently the case, all the tools are specified in the requirements.txt file, meaning there is no separation between the development and the production dependencies. This could be changed in the future but I do not think it is worth the (relatively simple but still existing) complexity for now.

Follow-up work

Replace Flake8 by Ruff in GitHub actions and remove the Flake8 dependency (EDIT: Done in this PR).

maximemulder · 2024-09-04T15:08:15Z

The strict configuration will use Pyright for now, but here is the equivalent Mypy configuration in case we want to change (or use both !).

[tool.mypy]
files = [
    "python/lib/db",
    "python/lib/exception",
]
strict = true

maximemulder · 2024-09-06T17:15:18Z

OKAY, I think this is done ! Not only does this PR configure the tooling, but it also adds GitHub actions related to this tooling. I made sure the outputs for Ruff and Pyright are formatted so that GitHub recognizes the errors, and shows them directly in the code. Deprecated calls are reported as warnings as we discussed. The dev and normal dependencies are not separated from the normal ones but this may be a subject for another PR (not urgent at all IMO).

Since our current Flake8 action checked for both Python 3.11 and 3.12, I did the same here. I can change that if you want a simplified workflow (it is nice that it may be theoretically more exhaustive and runs in parallel, but it also means errors can be reported twice).

maximemulder · 2024-09-09T13:32:07Z

Actually, I may have thought of a better way to do things this week-end (by containerizing the actions in a Docker image), let me see if I can succeed in another PR before merging.

* LORIS-MRI tooling configuration (aces#1170) * add pyproject.toml * keep flake8 (for now) * double type checking * Use Pyright as main type checker * remove comment * test * test * change configuration * add python environment variables * try factorization * test * test * test * test * remove temporary things * test ruff error * ruff output github * further test errors * test pyright output github * test * test * test * change comment * remove test errors * test --------- Co-authored-by: Maxime Mulder <maxime.mulder@mcgill.ca> * update docker install (aces#1171) Co-authored-by: Maxime Mulder <maxime.mulder@mcgill.ca> * add RB override tables for the MRI side (aces#1173) * Remove php 8.1 and 8.2 from the LORIS tests to keep only test on PHP 8.3 (aces#1175) * remove php 8.1 and 8.2 testing * update to 8.3 in Dockerfile.test.php8 * ADD minc tools install in gitactions (aces#1179) * todo install loris mri * test * run pytest in gitaction (aces#1172) * test * ttt * test * test * test * test * test * test * test * test * test * test * test * test * test * test * test * test * test * Delete test_example.py * rename * done * Delete test/Dockerfile.test.py * keep flake8 (for now) * double type checking * Use Pyright as main type checker * test * test * change configuration * add python environment variables * try factorization * test * test * test * remove temporary things * test ruff error * ruff output github * further test errors * test pyright output github * test * test * test * change comment * remove test errors * test * test * test * test * test * test * test * test cache * test * disable unused workflow * test * test * test * update repo owner in action * try add permissions * permissions test 2 * permissions check 3 * test * other test * use correct username * change login * try remove permission --------- Co-authored-by: Maxime Mulder <maxime.mulder@mcgill.ca> Co-authored-by: Cécile Madjar <cecile.madjar@gmail.com> Co-authored-by: Shen <kongtiaowangshen@gmail.com>

* remove trailing whitespaces (#1149) Co-authored-by: Maxime Mulder <maxime.mulder@mcgill.ca> * SQLAlchemy proof-of-concept (#1152) * add sqlalchemy * fix lints * add comment --------- Co-authored-by: Maxime Mulder <maxime.mulder@mcgill.ca> * fix (#1168) Co-authored-by: Maxime Mulder <maxime.mulder@mcgill.ca> * integration test (#1138) * test * main Loris here * ignore Loris python * clone Loris * rm loris * override test * override test * override RB data * fix folder * test php files * test * try * test * try install loris mri * os * test * rm modules * Update flake8_python_linter.yml * Small linting improvements (#1167) * better linting * remove tab in comment --------- Co-authored-by: Maxime Mulder <maxime.mulder@mcgill.ca> * Improve Python subject determination and validation error handling (#1146) * improve subjects determination and validation * use tuple for database query arguments * query on single line --------- Co-authored-by: Maxime Mulder <maxime.mulder@mcgill.ca> * Add literal regex leading r (#1176) Co-authored-by: Maxime Mulder <maxime.mulder@mcgill.ca> * fix (#1178) Co-authored-by: Maxime Mulder <maxime.mulder@mcgill.ca> * LORIS-MRI tooling configuration (#1170) * add pyproject.toml * keep flake8 (for now) * double type checking * Use Pyright as main type checker * remove comment * test * test * change configuration * add python environment variables * try factorization * test * test * test * test * remove temporary things * test ruff error * ruff output github * further test errors * test pyright output github * test * test * test * change comment * remove test errors * test --------- Co-authored-by: Maxime Mulder <maxime.mulder@mcgill.ca> * update docker install (#1171) Co-authored-by: Maxime Mulder <maxime.mulder@mcgill.ca> * add RB override tables for the MRI side (#1173) * Remove php 8.1 and 8.2 from the LORIS tests to keep only test on PHP 8.3 (#1175) * remove php 8.1 and 8.2 testing * update to 8.3 in Dockerfile.test.php8 * ADD minc tools install in gitactions (#1179) * todo install loris mri * test * run pytest in gitaction (#1172) * test * ttt * test * test * test * test * test * test * test * test * test * test * test * test * test * test * test * test * test * Delete test_example.py * rename * done * Delete test/Dockerfile.test.py * test (#1187) * unit tests poc (#1188) * add more pytest global rules (#1194) * Stricter linter configuration (#1193) * stricter ruff config * do not ignore N818 * lint for sorted imports * add python scripts directory (#1196) * Bunch of SQLAlchemy definitions (#1192) * bunch of sqlalchemy definitions * add project orm definition * fix candidate to site query function * optimize docker image (#1195) * fix pytest config (#1197) * fix python scripts (#1199) * remove mysql-connector (#1200) * Improve subject configuration structure (#1150) * refacor subject config * remove suspicious phantom code * change naming * fix rebase lints * cleaner database url * Cecile CandID path join bug fix Co-authored-by: Cécile Madjar <cecile.madjar@mcin.ca> --------- Co-authored-by: Cécile Madjar <cecile.madjar@mcin.ca> * Integration test to check that the ORM definitions match the LORIS SQL database (#1198) * orm sync integration test * add docker health check * use integration config file * test error * Revert "test error" This reverts commit d1a70e6. * Clean up Docker database scripts (#1202) * clean docker database * remove docker warning * remove unused attribute (#1201) * try fix perl ci * Add file, file_parameter and scanner SQLAlchemy models (#1207) * add tables * rename file model * rename parameter_file * fix lints (#1210) * Unify LORIS-MRI logging (#1191) * new env * change print order in database log * create notification type if needed * fix typing pyright update * Integration test for `run_dicom_archive_loader.py` (#1203) add integration test put keys in github repo debugging add database check add comments and debugging remove s3 keys change converter in test check file tree fix doc fix docs 2 rename variable go back to using s3 keys fix path try fix * rename database sub-namepaces (#1214) * fix warnings (#1215) * Factorize get subject session (#1190) * factorize subject session * re-add site log * Migrate "get all sites from the database" to the SQLAlchemy abstraction (#1216) * Fix new PEP585 Ruff lint (#1218) * Clean-up database scan type (#1139) Complementary PR of LORIS #9304 * Migrate DICOM archive files to new database abstraction (#1217) * wip * use self.dicom_archive.id instead of self.tarchive_id * fix todo * Fix SQLAlchemy file model (#1221) * Improve Python requirements order (#1222) * add python documentation (#1223) * Update SQLAlchemy bindings to match LORIS#9556 (#1229) * trigger_tests * try update * fix old queries for integration tests * Migrate `ImagingUpload` and `MriUploadDB` to the new database abstraction (#1224) * migrate mri upload to sqlalchemy * fix concurrency bug * fix clean up * display mri upload ids use dicom archive id instead of tarchiveid because it is prettier ruff check * migrate scanner to new abstraction (#1232) * Fix install script (#1234) * fix install script and move files related to install into the install directory * fix install script and move files related to install into the install directory * fix install script and move files related to install into the install directory * fix paths in imaging_install_test.sh * fix paths in imaging_install_test.sh * fix SQL error in install script * fix tests * fix tests * fix github action * add cpanfile * copy cpanfile in MRI dockerfile * add new dependency in order to be able to source the Digest::BLAKE2 in the cpanfile * fix Digest::BLAKE2 installation though cpanfile * move config templates to a templates directory and requirements files into a requirements directory * remove copy * fix test * fix test * fix test * fix test * Maxime's feedback * specify python version (#1237) * fix large file hash memory (#1236) * add lib.util module (#1238) * New DICOM study import script (#1117) * rewrite dicom archive * update * remove --year and --target options * get session from config file * remove unused upload argument * fix little oopsie for --session * handle large files hash * fix unhandled modalities * use new lib.util module * Add support to be able to read headers from enhanced DICOMs (#7) * print * print * print * print * add support for enhanced DICOMs * add support for enhanced DICOMs * rework associate files with series * sort dicom series and files before inserting in the database * handle null scanner --------- Co-authored-by: Cécile Madjar <cecile.madjar@mcin.ca> * refactoring of CandID on the non-ORM python side * tests * fix insertion in mri_protocol_violation_scans * alphabetical order is hard sometimes * FIx `get_candidate_id` documentation --------- Co-authored-by: Maxime Mulder <maxime-mulder@outlook.com> Co-authored-by: Maxime Mulder <maxime.mulder@mcgill.ca> Co-authored-by: Shen <kongtiaowangshen@gmail.com>

cmadjar assigned maximemulder Aug 30, 2024

maximemulder mentioned this pull request Sep 3, 2024

Remove commented PyBIDS code ? #1177

Open

maximemulder force-pushed the 2024-08-26_add-pyproject branch from 260116f to 2a973c5 Compare September 3, 2024 14:56

maximemulder force-pushed the 2024-08-26_add-pyproject branch from 3efa196 to 5e75fa9 Compare September 4, 2024 15:27

maximemulder mentioned this pull request Sep 4, 2024

Fix lints in bidsreader.py #1178

Merged

maximemuldermcgill added 6 commits September 6, 2024 09:56

add pyproject.toml

f375360

keep flake8 (for now)

b8727ca

double type checking

4cc3bae

Use Pyright as main type checker

6feea52

remove comment

6a79f7c

test

2d639f8

maximemulder force-pushed the 2024-08-26_add-pyproject branch from 5cf4673 to 2d639f8 Compare September 6, 2024 13:56

maximemuldermcgill added 17 commits September 6, 2024 09:59

test

42b0187

change configuration

e7e531e

add python environment variables

8011638

try factorization

d4673ab

test

a163069

test

1e4e94a

test

43cda03

test

88e9c19

remove temporary things

c5bbd9e

test ruff error

e53ea49

ruff output github

954a584

further test errors

b309b97

test pyright output github

d0b07b1

test

833d532

test

69580b5

test

a5558db

change comment

78729df

remove test errors

99d5ce6

maximemulder requested review from laemtl and cmadjar September 6, 2024 17:06

test

924f1f9

maximemulder marked this pull request as ready for review September 6, 2024 17:15

maximemulder mentioned this pull request Sep 9, 2024

LORIS-MRI tooling configuration (Docker version) #1182

Closed

laemtl added the Area: CI PR or issue related to continuous integration, including automated tests and static checks label Sep 13, 2024

cmadjar approved these changes Sep 13, 2024

View reviewed changes

cmadjar merged commit 72634dd into aces:main Sep 13, 2024
9 checks passed

cmadjar added this to the 27.0 milestone Sep 13, 2024

cmadjar added this to the 27.0 milestone Apr 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

LORIS-MRI tooling configuration #1170

LORIS-MRI tooling configuration #1170

Uh oh!

maximemulder commented Aug 30, 2024 •

edited

Loading

Uh oh!

maximemulder commented Sep 4, 2024 •

edited

Loading

Uh oh!

maximemulder commented Sep 6, 2024

Uh oh!

maximemulder commented Sep 9, 2024

Uh oh!

Uh oh!

Uh oh!

LORIS-MRI tooling configuration #1170

LORIS-MRI tooling configuration #1170

Uh oh!

Conversation

maximemulder commented Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Description

Location of the pyproject.toml file

Ruff

Pyright

Pytest

Dependencies in pyproject.toml

Dev dependencies vs prod dependencies

Follow-up work

Uh oh!

maximemulder commented Sep 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maximemulder commented Sep 6, 2024

Uh oh!

maximemulder commented Sep 9, 2024

Uh oh!

Uh oh!

Uh oh!

maximemulder commented Aug 30, 2024 •

edited

Loading

Location of the `pyproject.toml` file

Dependencies in `pyproject.toml`

maximemulder commented Sep 4, 2024 •

edited

Loading